[ENH] SQL: save user name and password via credentials manager #2403
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2403 +/- ##
=========================================
- Coverage 74.33% 74.3% -0.03%
=========================================
Files 321 321
Lines 55988 56011 +23
=========================================
+ Hits 41616 41621 +5
- Misses 14372 14390 +18 |
|
Not sure how this is supposed to work since username/password depend on the server/database. To reproduce, create a workflow that connects to two databases using different credentials. Save it, reload it and see what happens. |
|
@astaric: Well, besides username and password also server name/address/other could be saved. And not just one, more of them. |
Orange/widgets/data/owsql.py
Outdated
| @@ -50,12 +51,15 @@ class OWSql(OWWidget): | |||
| want_main_area = False | |||
| resizing_enabled = False | |||
|
|
|||
| cm_sql_username = CredentialManager("SQL username") | |||
| cm_sql_password = CredentialManager("SQL password") | |||
There was a problem hiding this comment.
Could this be, as per CredentialManager docstring, a single cm = CredentialManager('SQL Table') line, further below used as self.cm.username, self.cm.password?
There was a problem hiding this comment.
What about "SQL Table: {}:{}".format(self.host, self.port)
|
In that case, What's the issue with Settings again? |
Error reporting sends workflow with that information. |
jerneju
changed the title
|
Looks ok to me. Could you also add a migration that moves credentials from widget to Credentials Manager (it will be used when loading an old workflow). Bonus points if you add some tests :) |
Orange/widgets/data/owsql.py
Outdated
| @@ -55,8 +55,8 @@ class Outputs: | |||
| port = Setting(None) | |||
| database = Setting(None) | |||
| schema = Setting(None) | |||
| username = "" | |||
| password = "" | |||
| username = Setting(None) | |||
There was a problem hiding this comment.
What about adding a migrate_settings (class) method and storing the username and password to credentials manager there?
Issue
User name and password should not be saved in Settings.
Description of changes
Use credentials manager.
Includes